Skip to content

Add analyzer section in project.assets.json file#7464

Open
martinrrm wants to merge 4 commits into
devfrom
dev-mruizmares-analyzer-assets
Open

Add analyzer section in project.assets.json file#7464
martinrrm wants to merge 4 commits into
devfrom
dev-mruizmares-analyzer-assets

Conversation

@martinrrm

@martinrrm martinrrm commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Bug

Progress: NuGet/Home#6279
SDK PR (draft): dotnet/sdk#54646

Description

Today project.assets.json does not record which analyzers a package contributes, and analyzers are applied regardless of PrivateAssets / ExcludeAssets / IncludeAssets. This PR implements the NuGet restore half of the "indicate analyzer assets in project.assets.json" feature (spec: NuGet/Home#14455). The SDK consumption half ships as a separate dotnet/sdk PR.

When a project opts in with <RestoreEnableAnalyzerAssets>true</RestoreEnableAnalyzerAssets>, restore now:

  • Emits an analyzers group under each package in the assets file, listing every analyzer assembly. Detection uses a new AnalyzerAssemblies pattern in ManagedCodeConventions (any .dll under analyzers/ at any depth, excluding satellite .resources.dll), so the layout isn't assumed to be fixed and analyzer discovery is shared with the rest of the content model.
  • Respects asset filtering. PrivateAssets, ExcludeAssets, and IncludeAssets filter analyzers like any other asset type. Analyzers excluded or transitively suppressed are written as a _._ placeholder (consistent with compile / runtime / native).
  • Attaches selection metadata to each entry: codeLanguage (cs/vb/fs/any) and, when present in the path, compilerApiVersion (roslynX.Y) — mirroring how content files carry codeLanguage — so the SDK selects applicable analyzers from metadata instead of parsing paths. The metadata is derived by scanning the path segments, since real packages place the language at a variable depth (for example analyzers/dotnet/roslynX.Y/cs/).

Gating. The feature is opt-in and only honored for projects targeting .NET 11 or greater. The .NET 11+ gate is applied at evaluation time in NuGet.targets (mirroring NuGetAuditMode), and the resulting value is honored per target framework: it flows through TargetFrameworkInformation.RestoreEnableAnalyzerAssets and is read in code by every restore entry point — CLI/DG-spec (MSBuildRestoreUtility), VS nomination (PackageSpecFactory), and static-graph restore (MSBuildStaticGraphRestore, which previously did not honor the property at all). A multi-targeted net10.0;net11.0 project therefore emits the analyzers group only for net11.0.

Telemetry. ProjectRestoreInformation reports AnalyzerAssets.Enabled (opt-in state) and AnalyzerAssets.Count (number of analyzer assets emitted, excluding _._ placeholders). Richer adoption/impact telemetry to inform the default-on rollout ships as a follow-up.

Public API & serialization. Adds TargetFrameworkInformation.RestoreEnableAnalyzerAssets, LockFileTargetLibrary.AnalyzerAssets, LockFileItem.CompilerApiVersionProperty, and the ManagedCodeConventions.ManagedCodePatterns.AnalyzerAssemblies pattern (with the AnalyzerAssembly property name). The Newtonsoft.Json and System.Text.Json paths round-trip the new analyzers group and its metadata; the per-framework restore property round-trips through PackageSpecWriter and the streaming reader. The lock-file cache key includes the per-framework flag so toggling it invalidates correctly.

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

@martinrrm martinrrm requested a review from a team as a code owner June 8, 2026 23:06
@martinrrm martinrrm requested review from kartheekp-ms and zivkan June 8, 2026 23:06
jebriede
jebriede previously approved these changes Jun 9, 2026

@jebriede jebriede left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with a few suggestions.

Comment thread src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs Outdated
private static bool IsAnalyzerAssetPath(string path)
{
return path.StartsWith("analyzers/", StringComparison.Ordinal)
&& path.EndsWith(".dll", StringComparison.OrdinalIgnoreCase)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "analyzers/" prefix check is StringComparison.Ordinal (case-sensitive) while the .dll / .resources.dll checks are OrdinalIgnoreCase. If a package authored the folder as Analyzers/ this would skip it while the SDK's discovery (which we're mirroring) may still pick it up. Was case-sensitivity on the folder segment intentional? If not, consider OrdinalIgnoreCase for consistency with the extension checks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/LockFileUtils.cs Outdated
Comment thread src/NuGet.Core/NuGet.ProjectModel/LockFile/LockFileFormat.cs
jebriede
jebriede previously approved these changes Jun 15, 2026

@jebriede jebriede left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with an optimization suggestion.


telemetry.TelemetryEvent[UpdatedAssetsFile] = restoreResult._isAssetsFileDirty.Value;
telemetry.TelemetryEvent[UpdatedMSBuildFiles] = restoreResult._dirtyMSBuildFiles.Value.Count > 0;
telemetry.TelemetryEvent[AnalyzerAssetsCount] = CountAnalyzerAssets(assetsFile);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CountAnalyzerAssets runs on every real restore and walks every target and every library to populate AnalyzerAssetsCount. When the feature is off (which is the default and the common case during rollout) the count is always 0, so we pay for the full nested walk to produce a constant. Is it worth short-circuiting when the opt-in is disabled? Something like:

Suggested change
telemetry.TelemetryEvent[AnalyzerAssetsCount] = CountAnalyzerAssets(assetsFile);
telemetry.TelemetryEvent[AnalyzerAssetsCount] =
(_request.Project.RestoreMetadata?.RestoreEnableAnalyzerAssets ?? false)
? CountAnalyzerAssets(assetsFile)
: 0;

The inner walk is cheap per library (frozen libraries return Array.Empty), so this is a minor optimization rather than a correctness concern.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to make some changes here (like you said it always return 0) for the Telemetry, but we always want this since it will help us decide when to make this new behavior as default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a Telemetry spec for the feature https://github.com/NuGet/Client.Engineering/pull/3855

@nkolev92 nkolev92 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're introducing a new implementation pattern in a couple of different places, but we already have an established one.

<Output TaskParameter="AbsolutePaths" PropertyName="RestoreOutputAbsolutePath" />
</ConvertToAbsolutePath>

<ItemGroup Condition="'$(TargetFrameworks)' != ''">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we instead follow the logic for audit and pruning where the decision is made in code.

This implementation is only really a factor in standard CLI restore.
The other 3 restores share an implementation, this doesn't.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, and good catch!

return lockFileItems;
}

private static bool IsAnalyzerAssetPath(string path)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be using ManagedCodeConventions here.

That's what knows how to find the patterns.

NativeLibraries = new PatternSet(
conventions.Properties,
groupPatterns: new PatternDefinition[]
{
new PatternDefinition("runtimes/{rid}/nativeassets/{tfm}/{any?}", table: DotnetAnyTable),
new PatternDefinition("runtimes/{rid}/native/{any?}", table: null, defaults: DefaultTfmAny)
},
pathPatterns: new PatternDefinition[]
{
new PatternDefinition("runtimes/{rid}/nativeassets/{tfm}/{any}", table: DotnetAnyTable),
new PatternDefinition("runtimes/{rid}/native/{any}", table: null, defaults: DefaultTfmAny)
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an AnalyzerAssemblies PatternSet to ManagedCodeConventions (a terminal {analyzerAssembly}  token matching any .dll  under  analyzers/ at any depth, excluding .resources.dll satellites) and removed the hand-rolled IsAnalyzerAssetPath .

GetAnalyzerLockFileItems now consumes  contentItems.FindItems(...). I kept the metadata derivation (codeLanguage / compilerApiVersion) as a segment scan rather than content-model patterns, because the pattern engine extracts properties at fixed positions and the language segment isn't at a fixed position in practice.

I sampled 12 popular analyzer packages (67 analyzer DLLs): 66% put the language after the compiler version (analyzers/dotnet/roslynX.Y/cs/…), with the language appearing at depth 1, 2, or 3 depending on the package. Expressing that in the content model would need an enumerated set of depth-specific patterns plus new parsers and would silently mis-tag anything deeper. The scan handles all positions uniformly. Happy to revisit if you'd prefer the pattern approach.

/// the group has items. Empty groups are left alone.
/// </summary>
private static void ClearIfExists<T>(IList<T> group, Func<string, T> factory) where T : LockFileItem
private static void ClearIfExists<T>(IList<T> group, Func<string, T> factory, bool copyProperties = true) where T : LockFileItem

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what are the properties we're stopping from copying in the analyzers scenario?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The analyzer LockFileItems carry two metadata properties codeLanguage (cs / vb / fs / any) and, when present, compilerApiVersion  ( roslynX.Y )

int segmentLength = separator - segmentStart;
ReadOnlySpan<char> segment = path.AsSpan(segmentStart, segmentLength);

if (segment.Equals("cs".AsSpan(), StringComparison.OrdinalIgnoreCase))

@kartheekp-ms kartheekp-ms Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hard coded strings still cause allocations. Is there any way to avoid them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "cs" / "vb" / "fs" literals are interned by the compiler, so comparing against them — including "cs".AsSpan() over the literal doesn't allocate (added a comment noting this).

@dotnet-policy-service dotnet-policy-service Bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jun 25, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@martinrrm martinrrm removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jun 25, 2026
martinrrm and others added 3 commits June 25, 2026 16:48
…ventions detection

Move the RestoreEnableAnalyzerAssets opt-in gating into code, per target
framework. The value now flows through TargetFrameworkInformation and is read
by every restore entry point, including static-graph restore which previously
ignored the property entirely. Removes the project-level
ProjectRestoreMetadata.RestoreEnableAnalyzerAssets and the NuGet.targets OR.

Detect analyzers via a new AnalyzerAssemblies pattern in ManagedCodeConventions
instead of the hand-rolled IsAnalyzerAssetPath, so detection is shared with the
content model and matches the analyzers folder case-insensitively.

Clarify the analyzer metadata derivation comment and trim an allocation in the
compiler-version normalization.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@martinrrm martinrrm force-pushed the dev-mruizmares-analyzer-assets branch from 6d6559e to 963cbee Compare June 26, 2026 00:05
Add five properties to the ProjectRestoreInformation event to size the impact
of enabling RestoreEnableAnalyzerAssets by default before the rollout:

- AnalyzerAssets.Excluded.Count
- AnalyzerAssets.PackagesWithAnalyzers.Count
- AnalyzerAssets.PackagesWithExcludedAnalyzers.Count
- AnalyzerAssets.ExcludedByPrivateAssets.Count
- AnalyzerAssets.ExcludedByExcludeAssets.Count

The counts are derived from the resolved dependency graphs and the package
file lists, so they are reported on every full restore regardless of whether
the feature is currently enabled. This lets us measure how many packages and
analyzer assemblies would stop being applied once PrivateAssets/ExcludeAssets
are honored. AnalyzerAssets.Count is redefined as the number of analyzer
assemblies that would apply (computed the same way), replacing the previous
count of analyzer assets written to the assets file.

Analyzer detection mirrors ManagedCodeConventions.ManagedCodePatterns.
AnalyzerAssemblies as a cheap string check, avoiding a content-item collection
allocation per package on the restore path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@martinrrm martinrrm force-pushed the dev-mruizmares-analyzer-assets branch from 963cbee to b7099ed Compare June 26, 2026 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants